I believe switch expressions / statements have to be exhaustive or it is gong to be a compile time error. The code isn't going to compile if there's an unhandled case. IOW the runtime error will never be reachable, unless you add a default case. But adding a new case to an existing enum should require a major version bump I think? So we don't really have to handle that at runtime.

Also this could just be an expression:

return switch (colorScheme) { 
  null => null,
  .light => .light,
  .dart => .dart,
  .followSystem => .followSystem,
};

Actually, why do we need 2 enums that represent the same thing? Can we just re-export one / making one of the an alias of the other? Then we don't need this since there's only one type under the hood.

I believe switch expressions / statements have to be exhaustive or it is gong to be a compile time error.

Switch expressions do, not arbitrary switch statements.

But adding a new case to an existing enum should require a major version bump I think?

We have not traditionally considered that to be a major version change. We may need to revisit that at some point as switch expressions become more common, but for now we don't (at least for platform interfaces).

So we don't really have to handle that at runtime.

We do, for the specific goal of not requiring breaking the platform interface all the time.

Actually, why do we need 2 enums that represent the same thing?

Because Pigeon definitions currently aren't allowed to import things from other files. And doing so in the case of an enum from another package would be extremely dangerous since it would allow for Dart code to change without the native side changing.

Thanks both!

_platformMapColorSchemeFromMapColorScheme
The structure here mirrors _platformMarkerTypeFromMarkerType immediately above (line 1360).

I gather from the discussion above that the alternatives suggested (switch expression / collapsing the two enums) wouldn't fit here — the runtime fallback is intentional so new platform-interface enum values don't force a breaking change, and the Pigeon enum has to stay separate since Pigeon definitions can't import across files.

If there is something needed on my part or some restructuring needed please let me know.

I believe switch expressions / statements have to be exhaustive or it is gong to be a compile time error.

Switch expressions do, not arbitrary switch statements.

It turns out I was wrong here; some switch statements do have to be exhaustive now, including enums. The behavior changed from warning to error for enums several years ago and I'd never hit it, so we'll need to audit all the plugins for this construction and adjust it.

If there is something needed on my part or some restructuring needed please let me know.

Given what I found and described in the comment above, this should become a default case (with the ignore updated accordingly, and the part of the comment about not using default removed) so that it works as intended.

Done — switched to a default: case with the same ignore, and dropped the "deliberately outside the switch" note. Synced across all four iOS packages.

Should this method handle unknown. What do you want the unknown behavior to be an why.

I just followed patterns that are already established in the code so I am pretty confident about this.

// Line 370
static int toMapType(@NonNull PlatformMapType type) {
  return switch (type) {
    case NONE -> MAP_TYPE_NONE;
    case NORMAL -> MAP_TYPE_NORMAL;
    case SATELLITE -> MAP_TYPE_SATELLITE;
    case TERRAIN -> MAP_TYPE_TERRAIN;
    case HYBRID -> MAP_TYPE_HYBRID;
  };
}

// Line 388
static MapsInitializer.Renderer toMapRendererType(@Nullable PlatformRendererType type) {
  // null-check, then…
  return switch (type) {
    case LATEST -> MapsInitializer.Renderer.LATEST;
    case LEGACY -> MapsInitializer.Renderer.LEGACY;
  };
}

I think instead of tests that verify the 3 conversions I think the tests should be higher value like looping over all of one type and making sure there is a value that is of the appropriate other type. Handling unknown values etc.

Good point. I replaced the tests with a single loop over PlatformMapColorScheme.values(). The expected mapping is defined via an exhaustive switch expression, so a new enum value forces a compile error in both the production conversion and this test.

This needs a default case (with an ignore to suppress the analyzer warnings, and a comment similar to what's on like 1214, minus the part about not using default) to avoid potential future breakage (per this thread).

Done — added the default: case with // ignore: no_default_cases, unreachable_switch_default and a comment matching _platformMapTypeFromMapType (~line 1214), minus the part about not using default.

We generally consider new functionality to be a minor version bump.

Bumped to a minor version, 2.20.0.

If there is something needed on my part or some restructuring needed please let me know.

Given what I found and described in the comment above, this should become a default case (with the ignore updated accordingly, and the part of the comment about not using default removed) so that it works as intended.

Same versioning note for the iOS packages.

Bumped the iOS packages to minor — ios/sdk9/sdk10 are all 2.19.0.

Four distinct tests should be four distinct test functions, not a constant cycle of setup/assert/setup/assert in a single test.